This is my decisions and how I planned implementing this with Claude. Comments are welcome!

https://gist.github.com/matinzd/5cac3ef1811efc9817f055b8d27692f7

Not sure how to update ReactAndroid.api tho? Are there any scripts or gradle tasks to do this? @cortinico

@matinzd we can generate it after importing, I don't believe there's a public Gradle script for it

we can generate it after importing, I don't believe there's a public Gradle script for it

Yep. I saw the command in other PRs using buck2!

The PR is now ready to review. Can you please approve the jobs to run?

Thanks for tackling this @matinzd, the ActivityResultContract gap is real and Health Connect is a good example.

A few things I think are worth sorting first, mostly around lifecycle and threading, though I might be missing some context:

1. Threading. As far as I can tell a TurboModule field initializer and Promise bodies run on the JS or native-modules thread rather than the UI thread, but ActivityResultRegistry.register and onLaunch are @MainThread and touch non-thread-safe maps. It looks like register (ReactActivityResultCallerImpl.kt) and launch (DeferredActivityResultLauncher.kt) run on the caller thread today, so I think this could crash or misbehave depending on the device. I think both would need to hop to the UI thread, unless I am misreading where they run.

2. Rebinding to the current Activity. It seems the launcher rebinds only when it is not already bound and does not check whether the bound registry is still current. With multi-activity nav, or the order where the new activity's onHostResume fires before the old one's onHostDestroy, I think it stays bound to the old dead registry. onHostDestroy for the old activity also seems to be skipped by the activity === currentActivity guard, so unbind may never run. If that is right, a launch from the new screen would dispatch into the old activity, results land in the wrong place and the old activity leaks. The single-activity config-change path probably does not hit this since it is the serial order. I think onHostResume might need to always rebind to the current registry rather than gate on isBound.

3. Keying by the contract class name. It seems two libraries both using a stock contract like GetContent or RequestPermission would get the same key, so the second register throws at field-init and that whole module fails to construct. I think AndroidX auto-generates keys for this reason. The owner-scoped overload helps but the default steers everyone into the collision. This one feels like a design call, so curious what you and the core team think, maybe default to owner-scoped or a caller-provided key with the FQCN as opt-in.

On testing, it looks like the current coverage is mostly the sample module wiring, so the lifecycle and threading paths above are not really exercised. It would help to add tests for those paths, mainly the rebind case (a launch after an Activity swap should hit the new registry) and the main-thread requirement for register and launch.

Thanks for the review @fabriziocucci!

3. Keying by the contract class name. It seems two libraries both using a stock contract like GetContent or RequestPermission would get the same key, so the second register throws at field-init and that whole module fails to construct. I think AndroidX auto-generates keys for this reason. The owner-scoped overload helps but the default steers everyone into the collision. This one feels like a design call, so curious what you and the core team think, maybe default to owner-scoped or a caller-provided key with the FQCN as opt-in.

Sorry, that slipped through. Claude made a mistake, and it’s good that you caught it.

I’m thinking of deriving the key from the FQCN of the native module combined with the contract FQCN. We could throw an error if the same contract is registered twice from the same module, prompting users to use the overload that accepts a caller-provided key instead.

E.g:

// Throws: MyModule already registered a launcher for androidx...GetContent.
private val pickAvatar = ctx.registerForActivityResult(this, GetContent()) { }
private val pickBanner = ctx.registerForActivityResult(this, GetContent()) { }

// Fix:
private val pickAvatar = ctx.registerForActivityResult("avatar", GetContent()) { }
private val pickBanner = ctx.registerForActivityResult("banner", GetContent()) { }

or we can just silently add an index based key based on each module to avoid collision for e.g:

// key = "com.some.image.lib.ImageModule:androidx...GetContent#0"
// key = "com.some.image.lib.ImageModule:androidx...GetContent#1"

What do you think? (Not sure who to tag though from the core team)

On testing, it looks like the current coverage is mostly the sample module wiring, so the lifecycle and threading paths above are not really exercised. It would help to add tests for those paths, mainly the rebind case (a launch after an Activity swap should hit the new registry) and the main-thread requirement for register and launch.

For the rebind case, I tried adding a setTimeout locally when requesting a contract/permission, then used back button to close and put the app in the background. As soon as I opened the app the rebind happened and the picker showed up (Didn't check if the promise was resolved though because in that case the js context is already invalidated).

For the rest, I need to create an example app in order to test those cases. I will let you know when that's ready.